Codecov Report

❌ Patch coverage is 31.61765% with 93 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.81%. Comparing base (cdbe896) to head (cafe3db).
⚠️ Report is 37 commits behind head on main.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main   #20734      +/-   ##
==========================================
+ Coverage   47.44%   47.81%   +0.36%     
==========================================
  Files        4002     4008       +6     
  Lines      123116   123411     +295     
  Branches    18938    18973      +35     
==========================================
+ Hits        58414    59005     +591     
+ Misses      60277    59958     -319     
- Partials     4425     4448      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@claude Please review!

Claude finished @Banrion's task in 3m 54s —— View job

Claude Code Review

Reviewed the carousel signal migration, the new NewAdminWelcomeDialogComponent, the OnboardingService updates, the DevMenuComponent refactor, and the v1/v2 page integrations.

🔴 Bugs

1. v2 page's dev menu is wired to a removed output

bitwarden_license/bit-web/src/app/dirt/access-intelligence/v2/access-intelligence-page/access-intelligence-page.component.html:110

<dirt-dev-menu
  (beginTour)="beginOnboardingTour()"
  (importData)="goToImportPage()"
></dirt-dev-menu>

DevMenuComponent was refactored — beginTour was replaced by two new outputs (beginPostImportTour, beginNewAdminWelcomeTour). With strict template checking this binding fails to compile, and at minimum it's a no-op. The dev menu in v2 will not invoke any tour.

Also, while v1 (risk-insights.component.ts) was updated to launch NewAdminWelcomeDialogComponent from the empty/no-ciphers state, the v2 page (access-intelligence-page.component.ts) was not. Was the v2 wiring intentionally deferred? If not, the new-admin welcome flow won't fire on the v2 path at all.

Fix this →

2. Storybook mock uses the wrong service method name

bitwarden_license/bit-web/src/app/dirt/access-intelligence/onboarding/new-admin-welcome-dialog.component.stories.ts:20

const mockOnboardingService = { setCarouselAcknowledged: async () => {} };

The component calls setNewAdminWelcomeDialogAcknowledged() from both onSkip and onImportData. Pressing Skip or Import Data in the story throws setNewAdminWelcomeDialogAcknowledged is not a function.

Fix this →

🟡 Quality

3. TOTAL_SLIDES = 4 must stay manually in sync with the template — new-admin-welcome-dialog.component.ts:31. Adding or removing a <vault-carousel-slide> requires also bumping this constant, with no compile-time check. Consider deriving slide count via the carousel's slideChange (last index emitted) or by counting via a template-ref QueryList.

4. All four slides use the same placeholder image — new-admin-welcome-dialog.component.html lines 13, 30, 44, 58 all point at /images/access-intelligence/data-is-in.svg. If this is awaiting design, a TODO would help; otherwise the slides are visually indistinguishable.

5. Injected DIALOG_DATA is unused in PostImportModalDialogComponent — post-import-modal-dialog.component.ts:37 injects data and WelcomeModalDialogData but nothing in the component reads it. Either remove the injection (and the organizationId argument from showDialog) or use it.

6. Silent error swallowing in PostImportModalDialogComponent.onSkip — post-import-modal-dialog.component.ts:43-50 uses .catch(() => {}), so a failure to set acknowledged state is invisible. Even logging via LogService (as the dev menu does) would be enough. The new admin dialog's onSkip doesn't catch at all — pick one convention.

7. Test coverage regression for post-import dialog — The deleted welcome-modal-dialog.component.spec.ts covered onStartTour; the new post-import-modal-dialog.component.spec.ts only tests onSkip and creation. Re-add an onStartTour test.

8. Missing alt text — post-import-modal-dialog.component.html:4 <img src=".../data-is-in.svg"> has no alt. The new-admin dialog correctly uses alt="" aria-hidden="true" — apply the same here for consistency.

9. nextSlide/prevSlide widened from protected to public — carousel.component.ts:128,134. This expands the public API of a shared libs/vault component to satisfy a single consumer that calls them through a template ref. Acceptable, but worth confirming the carousel team is OK with the expanded surface (could alternatively be driven by parent-supplied [selectedIndex] input).

10. Minor — duplicate tw-text-center in new-admin-welcome-dialog.component.html on bitDialogContent and again on each slide's inner <div>. The outer one is sufficient.

11. Unused params argument — risk-insights.component.ts:413 (and the v2 mirror at :439) declare params: string[] and never use it. Pre-existing, but flagging since this PR touches these files.

✅ Strengths

• OnboardingService correctly splits the two acknowledgement keys and persists them via clearOn: [].
• combineLatest([hasReportData$, hasCiphers$]) + take(1) in risk-insights.component.ts:282-292 correctly guarantees the welcome tour fires at most once per page mount, and the acknowledged check inside showDialog short-circuits subsequent navigations.
• Nice signal-based refactor of currentSlide / isFirstSlide / isLastSlide in response to @jengstrom-bw's feedback.
• hideArrows correctly implemented as input<boolean>() rather than another @Input.

Summary: two bugs (v2 dev-menu event binding mismatch + story mock method name) should block; the rest are polish. The v2 page also seems to be missing the new-admin welcome wiring entirely — please confirm whether that's intentional.
• Branch: dirt/pm-34724/carousel-for-first-time-admins

Claude Code is reviewing this pull request...

If this comment does not update with results, check the Actions log.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.3% Duplication on New Code

See analysis details on SonarQube Cloud